Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve YAML parsing to handle list sequences within SLI spec #325

Closed
wants to merge 2 commits into from
Closed

Improve YAML parsing to handle list sequences within SLI spec #325

wants to merge 2 commits into from

Conversation

aaranmcguire
Copy link
Contributor

This PR will allow more YAML configurations within the SLI spec as lists were previously not parsed.

Within my org we're wanting to allow our users to see SLOs by cluster, or server. This is likely niche, but we're doing this by allowing users to define the Prometheus labels to scrape. e.g. sum(http_requests_total{}) by (cluster). To do this we're wanting the spec to look like the following.

indicator:
    spec:
      ratioMetric:
        total:
          metricSource:
            type: Prometheus
            metricSourceRef: thanos
            spec:
              query: http_requests_total{service="example"}
              dimensions:
                - cluster
                - server

This may be niche, but we believe that the spec was written to be open minded regarding the format within the metricSource.spec, so this should be supported.

FYI, the output of this within the Go struct matches the existing format with dimensions being cluster;server

@nieomylnieja
Copy link
Member

nieomylnieja commented Aug 26, 2024 •
edited
Loading

hey @aaranmcguire, thanks for the contribution!
this looks good, but linter reports some errors, please correct them :)

you might be interested in OpenSLO/OpenSLO#276, as this oslo code will be replaced by the openslo package. Would you mind contributing to the branch of my PR there as well?

@nieomylnieja nieomylnieja mentioned this pull request Aug 26, 2024
Merged
@aaranmcguire aaranmcguire force-pushed the aaran/support-lists-in-sli-spec branch from ed83de3 to a0df812 Compare August 27, 2024 10:22
nieomylnieja
nieomylnieja requested changes Aug 27, 2024
View reviewed changes
pkg/manifest/v1/indicator.go Outdated Show resolved Hide resolved
pkg/manifest/v1/alerts.go Outdated Show resolved Hide resolved
@aaranmcguire aaranmcguire force-pushed the aaran/support-lists-in-sli-spec branch from 27428a1 to f4ec60b Compare August 27, 2024 15:12
@aaranmcguire
Copy link
Contributor Author

I've really F'd this one up with the force push, let me just do it again from scratch.

@nieomylnieja
Copy link
Member

On seconds thoughts and inspection of the current, v1 schema description, I think oslo has at a wrong treating spec as a map[string]string, the OpenSLO specification states:

arbitrary chosen fields for every data source type to make it comfortable to use
anything that is valid YAML can be put here.

This means that the SLI spec should in fact be map[string]interface{}.

I've added these changes in the following OpenSLO commit:
OpenSLO/OpenSLO@c8354c5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nieomylnieja nieomylnieja nieomylnieja requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aaranmcguire @nieomylnieja